Repository navigation
fix(gl): report quickstart registration failure when the node returns no UCAN (#355) - #556
Conversation
… no UCAN (#355) A 2xx with a non-JSON or ucan-less body still printed "Registered successfully / UCAN saved to ..." while nothing was written, and left a stale ucan.json from another node in place. Print the success lines only when a UCAN was actually persisted.
|
@coderabbitai full review |
|
Warning Review limit reachedThis review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Next included review available in 13 minutes. View limit details
|
Action not completed
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Merge readiness
Rebase onto current main (a2262d92, reconciliation sweep #551) before merge. Merge-base is c805e35f; the branch is one commit behind the live target with no conflicts predicted (mergeable: MERGEABLE).
The PR template box "New behavior is covered by tests (required for fixes)" is checked, but the diff has no test changes and quickstart.rs has no unit tests. Verification is documented as a manual stub run only. That is acceptable for a stdout-only change if maintainers agree, but the checkbox overstates what landed in the PR—either add a mocked HTTP regression (same style as register.rs tests) or uncheck and cite manual verification explicitly.
CI: test (stable), fmt + clippy, build --release, and the rest required suite passed on head ebdecb8. test (beta) failed on the PR run; stable passed on the same workflow. Treat beta as merge-gating only if your branch protection requires it—confirm whether the failure reproduces on main or is toolchain flake.
Upstream duplicate: fork PR #458 (closed) carried the same fix; this branch correctly drops the no-UCAN retry hint so gl register is not suggested without this run's --dir.
🟢 Thanks for the contribution. I do not see any actionable issues from my review.
The change matches #355: on POST /api/register 2xx, success and “UCAN saved” lines run only inside the existing !ucan.is_empty() write guard; an empty or missing ucan prints a failure row instead of false success. Trust score output moves with that guard. Follow-ups called out in the issue (read_json instead of unwrap_or_default(), register.rs messaging, stale ucan.json lifecycle, wizard non-fatal Step 3) are pre-existing or explicitly scoped out of this single-file port—not regressions introduced by the diff.
Summary
Carries #458 (
fix(gl): report quickstart registration failure when the node returns no UCAN, for #355) onto an upstream branch so the upstream-only Code Quality check can run.Motivation & context
gl quickstartprinted "Registered successfully" whenever the node returned 2xx, even when the body carried no UCAN. The registration quietly did nothing and could leave a stale ucan.json from another node in place.Closes #355.
Kind of change
What changed
gl: a 2xx response without a usable UCAN now prints a failure line instead of a false success. Success output and the UCAN write happen only inside the non-empty-UCAN branch; the nonfatal continuation to repo creation is unchanged.a418c284; one merge commit bringsmainatc805e35f(no conflicts). The merge commit also resolves the outstanding review finding: the recovery hint on the new no-UCAN branch is removed rather than retained, because agl registerinstruction that drops this invocation's--dirwould load a different identity or a missing one entirely. The pre-existing hints on the non-2xx and transport-failure branches are unchanged.How a reviewer can verify
Run
gl quickstartagainst a node stub returning a 2xx body without aucanfield: the output ends at the failure line; no retry command is printed and no ucan.json is written.Before you request review
cargo test --workspacepasses locallycargo fmt --allandcargo clippy --workspace --all-targets -- -D warningsare cleanfeat(...),fix(...),docs(...)).env.exampleupdated if behavior or config changed (or N/A)Protocol & signing impact
did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formatsCLI output change only; no wire or identity handling changes.
Notes for reviewers